test/e2e-oidc/external_oidc.go (1)

441-470: Consider extending algorithm support for future-proofing.

The extractRSAPubKeyFunc only handles RS256. While this matches current Keycloak defaults, if the IDP configuration changes to use RS384/RS512, tests would fail with an unhelpful error. This is acceptable for now but worth noting.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/e2e-oidc/external_oidc.go` around lines 441 - 470, extractRSAPubKeyFunc
currently only accepts "RS256" and returns an error for other RSA algorithms;
update it to accept other RSA signing algorithms (e.g., "RS384", "RS512") or any
algorithm that starts with "RS" so the JWKS parsing is future-proof: modify the
switch on key.Alg in extractRSAPubKeyFunc (and the related issuerJWKS/Key usage)
to handle "RS384" and "RS512" (or use a strings.HasPrefix(key.Alg, "RS") check)
and keep constructing the rsa.PublicKey the same way, while keeping the
unexpected-algorithm error for non-RS algorithms. Ensure the error messages
still include key.KID and key.Alg for clarity.

test/e2e-encryption-perf/encryption_perf.go (1)

55-66: Simplify error handling with direct t.Fatal calls.

The pattern of creating an error and immediately passing it to require.NoError is confusing. Since the error is always non-nil when created, require.NoError will always fail, making this equivalent to t.Fatal but harder to read.

♻️ Proposed simplification

 		AssertDBPopulatedFunc: func(t testing.TB, errorStore map[string]int, statStore map[string]int) {
 			tokenCount, ok := statStore[tokenStatsKey]
 			if !ok {
-				err := errors.New("missing oauth access tokens count stats, can't continue the test")
-				require.NoError(t, err)
+				t.Fatal("missing oauth access tokens count stats, can't continue the test")
 			}
 			if tokenCount < 14000 {
-				err := fmt.Errorf("expected to create at least 14000 tokens but %d were created", tokenCount)
-				require.NoError(t, err)
+				t.Fatalf("expected to create at least 14000 tokens but %d were created", tokenCount)
 			}
 			t.Logf("Created %d access tokens", tokenCount)
 		},

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/e2e-encryption-perf/encryption_perf.go` around lines 55 - 66, In
AssertDBPopulatedFunc simplify error handling by replacing the pattern that
constructs an error and calls require.NoError with direct test failures: when
statStore[tokenStatsKey] is missing call t.Fatalf (or require.FailNow) with a
clear message about the missing oauth access tokens count, and when tokenCount <
14000 call t.Fatalf (or require.FailNow) with a message showing the expected vs
actual tokenCount; update references around AssertDBPopulatedFunc and
tokenStatsKey accordingly.

test/e2e-encryption-rotation/encryption_rotation.go (1)

47-75: Consider adding retry logic for the operator update.

The UnsupportedConfigFunc performs a read-modify-write operation without handling potential conflict errors. If the operator object is modified between the Get (line 49) and Update (line 73), the update will fail with a conflict. For a 3-hour test, this could cause intermittent failures.

If conflicts are expected to be rare in this test context, the current implementation is acceptable, but retry-on-conflict would improve reliability.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/e2e-encryption-rotation/encryption_rotation.go` around lines 47 - 75,
Wrap the read-modify-write in UnsupportedConfigFunc with retry-on-conflict logic
(e.g., use k8s.io/client-go/util/retry.RetryOnConflict) so that if
cs.OperatorClient.Update returns a conflict you re-get the Operator
(cs.OperatorClient.Get), reapply the unsupportedConfig merge steps and retry the
Update; ensure the loop returns the final error if retries are exhausted and
keep references to UnsupportedConfigFunc, cs.OperatorClient.Get, and
cs.OperatorClient.Update to locate the change.

test/e2e-encryption/encryption.go (1)

54-74: Consider reusing the provided ClientSet parameter.

The CreateResourceFunc callback receives a library.ClientSet parameter but ignores it, creating a new client via operatorencryption.GetClients(t). If this is intentional (e.g., needing operator-specific client configuration), adding a brief comment would clarify the design choice.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/e2e-encryption/encryption.go` around lines 54 - 74, The
CreateResourceFunc inside testEncryptionTurnOnAndOff ignores the provided
library.ClientSet parameter and calls operatorencryption.GetClients(t); either
update CreateResourceFunc to use the passed-in client (the second parameter)
when calling operatorencryption.CreateAndStoreTokenOfLife (or adapt/convert the
library.ClientSet to the expected client type), or if using GetClients(t) is
intentional, add a brief comment above CreateResourceFunc explaining why
operatorencryption.GetClients(t) is required instead of the supplied client;
reference CreateResourceFunc, operatorencryption.CreateAndStoreTokenOfLife, and
operatorencryption.GetClients for locating the change.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

test/library/encryption/scenarios.go (3)

340-349: ⚠️ Potential issue | 🟠 Major

Retry the APIServer update on conflict.

This is still a single read-modify-write against apiserver/cluster. If anything updates the object between Get and Update, the test fails on a transient resourceVersion conflict even though retrying would succeed. Wrap the mutation in retry.RetryOnConflict here.

Suggested fix

+	"k8s.io/client-go/util/retry"

-	apiServer, err := libClientSet.ApiServerConfig.Get(context.TODO(), "cluster", metav1.GetOptions{})
-	require.NoError(tb, err)
-
-	// Update encryption type if needed
-	needsUpdate := apiServer.Spec.Encryption.Type != encryptionType
-	if needsUpdate {
-		tb.Logf("Updating encryption type in the config file for APIServer to %q", encryptionType)
-		apiServer.Spec.Encryption.Type = encryptionType
-		_, err = libClientSet.ApiServerConfig.Update(context.TODO(), apiServer, metav1.UpdateOptions{})
-		require.NoError(tb, err)
+	apiServer, err := libClientSet.ApiServerConfig.Get(context.TODO(), "cluster", metav1.GetOptions{})
+	require.NoError(tb, err)
+
+	needsUpdate := apiServer.Spec.Encryption.Type != encryptionType
+	if needsUpdate {
+		tb.Logf("Updating encryption type in the config file for APIServer to %q", encryptionType)
+		err = retry.RetryOnConflict(retry.DefaultRetry, func() error {
+			apiServer, err = libClientSet.ApiServerConfig.Get(context.TODO(), "cluster", metav1.GetOptions{})
+			if err != nil {
+				return err
+			}
+			apiServer.Spec.Encryption.Type = encryptionType
+			_, err = libClientSet.ApiServerConfig.Update(context.TODO(), apiServer, metav1.UpdateOptions{})
+			return err
+		})
+		require.NoError(tb, err)
 	} else {
 		tb.Logf("APIServer is already configured to use %q mode", encryptionType)
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/library/encryption/scenarios.go` around lines 340 - 349, The update to
the APIServer object does a single read-modify-write and can fail on
resourceVersion conflicts; wrap the mutation+Update call in a
retry.RetryOnConflict loop so it re-reads and retries on conflict. Locate the
block that calls libClientSet.ApiServerConfig.Get and then sets
apiServer.Spec.Encryption.Type and calling libClientSet.ApiServerConfig.Update,
and replace that update path with a retry.RetryOnConflict that fetches the
latest ApiServer, applies the same encryptionType mutation to
apiServer.Spec.Encryption.Type, and retries the Update until it succeeds or a
non-conflict error occurs.

---

204-210: ⚠️ Potential issue | 🟠 Major

Don't use a non-blocking receive for migrationStartedCh.

If the watcher goroutine sends a moment later, this default path reports a false failure and never computes the migration duration. Wait on the same channel with a bounded timeout or cancelable context instead of falling through immediately.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/library/encryption/scenarios.go` around lines 204 - 210, The
non-blocking select on migrationStartedCh can cause false failures; replace the
select/default with a blocking receive that waits with a bounded timeout or a
cancelable context so the test actually observes the watcher send. Specifically,
wait for migrationStartedCh (instead of the default path) and compute
migrationTime = endTimeStamp.Sub(migrationStarted) before calling tb.Logf and
scenario.AssertMigrationTime; if the timeout/context expires, call tb.Error with
a clear timeout message. Use the existing symbols migrationStartedCh,
endTimeStamp, tb.Logf, and scenario.AssertMigrationTime to locate and update the
code.

---

73-81: ⚠️ Potential issue | 🟠 Major

Dispatch "" to TestEncryptionTypeUnset.

The empty-string provider still falls through to TestEncryptionTypeIdentity, which writes "identity" into APIServer.Spec.Encryption.Type instead of exercising the unset/defaulting path that this helper already models separately.

Suggested fix

 	switch provider {
 	case configv1.EncryptionTypeAESCBC:
 		TestEncryptionTypeAESCBC(tb, scenario)
 	case configv1.EncryptionTypeAESGCM:
 		TestEncryptionTypeAESGCM(tb, scenario)
 	case configv1.EncryptionTypeKMS:
 		TestEncryptionTypeKMS(tb, scenario)
-	case configv1.EncryptionTypeIdentity, "":
+	case configv1.EncryptionTypeIdentity:
 		TestEncryptionTypeIdentity(tb, scenario)
+	case "":
+		TestEncryptionTypeUnset(tb, scenario)
 	default:
 		tb.Fatalf("Unknown encryption type: %s", provider)
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/library/encryption/scenarios.go` around lines 73 - 81, The switch in the
dispatcher incorrectly routes the empty-string provider to
TestEncryptionTypeIdentity; update the case handling so that
configv1.EncryptionTypeIdentity maps only to TestEncryptionTypeIdentity and the
empty string ("") maps to TestEncryptionTypeUnset. Locate the switch in
TestEncryption scenarios (the switch on provider in scenarios.go) and replace
the combined case `case configv1.EncryptionTypeIdentity, "":
TestEncryptionTypeIdentity(tb, scenario)` with two separate cases: one calling
TestEncryptionTypeIdentity(tb, scenario) for configv1.EncryptionTypeIdentity and
one calling TestEncryptionTypeUnset(tb, scenario) for the "" provider; leave the
other cases (TestEncryptionTypeAESCBC, TestEncryptionTypeAESGCM,
TestEncryptionTypeKMS) unchanged.

test/library/encryption/perf_helpers.go (1)

35-46: ⚠️ Potential issue | 🟠 Major

Only capture a fresh "Migrating" transition.

This still records any EncryptionMigrationControllerProgressing=True condition, even if it predates the current test or has a different reason. That can inflate the reported migration duration or use a stale start time. Capture a start := time.Now() before polling and only send LastTransitionTime when the condition is True, Reason == "Migrating", and the transition is newer than start.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/library/encryption/perf_helpers.go` around lines 35 - 46, The poll
currently sends any EncryptionMigrationControllerProgressing condition with
Status True, possibly using a stale LastTransitionTime; fix by recording a start
:= time.Now() immediately before calling wait.PollUntilContextTimeout and inside
the poll only send cond.LastTransitionTime to migrationStartedCh when cond.Type
== "EncryptionMigrationControllerProgressing", cond.Status ==
operatorv1.ConditionTrue, cond.Reason == "Migrating" and
cond.LastTransitionTime.Time.After(start); use getOperatorConditionsFn to fetch
conditions and ensure you return true,nil only when the transition is newer than
start.

pkg/controllers/readiness/wellknown_ready_controller.go (1)

355-358: Optional: cache KAS service port once per function for readability.

Line 357 is correct, but pulling getKASServicePort() out of the loop makes intent clearer and avoids repeated calls in the condition.

♻️ Proposed refactor

 func getKASTargetPortFromService(service *corev1.Service) (int, bool) {
+	kasPort := getKASServicePort()
 	for _, port := range service.Spec.Ports {
-		if targetPort := port.TargetPort.IntValue(); targetPort != 0 && port.Protocol == corev1.ProtocolTCP && int(port.Port) == getKASServicePort() {
+		if targetPort := port.TargetPort.IntValue(); targetPort != 0 && port.Protocol == corev1.ProtocolTCP && int(port.Port) == kasPort {
 			return targetPort, true
 		}
 	}
 	return 0, false
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/controllers/readiness/wellknown_ready_controller.go` around lines 355 -
358, The loop in getKASTargetPortFromService repeatedly calls
getKASServicePort() inside the condition which obscures intent; cache the result
once before iterating by assigning kasPort := getKASServicePort(), then use
kasPort in the loop condition (referencing getKASTargetPortFromService and
getKASServicePort) so the code is clearer and avoids repeated calls.

test/e2e-encryption-perf/encryption_perf.go (2)

55-65: Consider using t.Fatalf directly instead of require.NoError with fabricated errors.

The current pattern creates an error just to pass it to require.NoError, which is indirect. Using t.Fatalf would be more idiomatic:

♻️ Suggested simplification

 AssertDBPopulatedFunc: func(t testing.TB, errorStore map[string]int, statStore map[string]int) {
 	tokenCount, ok := statStore[tokenStatsKey]
 	if !ok {
-		err := errors.New("missing oauth access tokens count stats, can't continue the test")
-		require.NoError(t, err)
+		t.Fatalf("missing oauth access tokens count stats, can't continue the test")
 	}
 	if tokenCount < 14000 {
-		err := fmt.Errorf("expected to create at least 14000 tokens but %d were created", tokenCount)
-		require.NoError(t, err)
+		t.Fatalf("expected to create at least 14000 tokens but %d were created", tokenCount)
 	}
 	t.Logf("Created %d access tokens", tokenCount)
 },

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/e2e-encryption-perf/encryption_perf.go` around lines 55 - 65, The
AssertDBPopulatedFunc currently fabricates errors and calls require.NoError;
change those to direct test failures using t.Fatalf: when tokenStatsKey is
missing, call t.Fatalf("missing oauth access tokens count stats, can't continue
the test"); when tokenCount < 14000, call t.Fatalf("expected to create at least
14000 tokens but %d were created", tokenCount). Update the block in
AssertDBPopulatedFunc (which reads tokenCount, ok := statStore[tokenStatsKey])
to remove the temporary err variables and use t.Fatalf with the appropriate
messages.

---

81-97: Unused namespace parameter in the returned function.

The namespace parameter on line 82 is unused. If this is intentional (OAuth access tokens are cluster-scoped), consider using _ to make it explicit that it's intentionally ignored for clarity, consistent with the other unused parameters.

♻️ Suggested clarification

 func createAccessTokenWrapper(ctx context.Context, tokenClient oauthclient.OAuthAccessTokensGetter) library.DBLoaderFuncType {
-	return func(_ kubernetes.Interface, namespace string, errorCollector func(error), statsCollector func(string)) error {
+	return func(_ kubernetes.Interface, _ string, errorCollector func(error), statsCollector func(string)) error {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/e2e-encryption-perf/encryption_perf.go` around lines 81 - 97, The
returned anonymous function in createAccessTokenWrapper currently declares a
parameter named namespace that is never used; update the signature of the
returned func (in createAccessTokenWrapper) to replace the unused namespace
parameter with _ (e.g., func(_ kubernetes.Interface, _ string, errorCollector
func(error), statsCollector func(string)) error) so it's explicit the namespace
is intentionally ignored while keeping the rest of the behavior the same.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

test/e2e-encryption-kms/encryption_kms.go (2)

48-53: Typo: "techincal" should be "technical".

Line 52 contains a spelling error in the comment.

📝 Fix typo

 	// TODO: Remove this skip once the authentication operator fully supports KMS encryption.
 	// Currently, while the API accepts encryption.type: "KMS" and the operator mounts the KMS
 	// plugin socket, it does not generate the EncryptionConfiguration with KMS provider stanza.
 	// This causes tests to timeout waiting for encryption keys to be created and migration to complete.
-	// See: https://redhat.atlassian.net/browse/OCPSTRAT-108 it is still under techincal preview
+	// See: https://redhat.atlassian.net/browse/OCPSTRAT-108 it is still under technical preview
 	t.Skip("Skipping KMS encryption test: operator implementation is incomplete")

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/e2e-encryption-kms/encryption_kms.go` around lines 48 - 53, Fix the
spelling mistake in the comment above the t.Skip call: change "techincal" to
"technical" in the comment that references the operator being under technical
preview (the comment block immediately preceding the t.Skip("Skipping KMS
encryption test: operator implementation is incomplete") line).

---

95-100: Same typo: "techincal" should be "technical".

📝 Fix typo

 	// TODO: Remove this skip once the authentication operator fully supports KMS encryption.
 	// Currently, while the API accepts encryption.type: "KMS" and the operator mounts the KMS
 	// plugin socket, it does not generate the EncryptionConfiguration with KMS provider stanza.
 	// This causes tests to timeout waiting for encryption keys to be created and migration to complete.
-	// See: https://redhat.atlassian.net/browse/OCPSTRAT-108 it is still under techincal preview
+	// See: https://redhat.atlassian.net/browse/OCPSTRAT-108 it is still under technical preview
 	t.Skip("Skipping KMS encryption test: operator implementation is incomplete")

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/e2e-encryption-kms/encryption_kms.go` around lines 95 - 100, Fix the
typo in the block comment above the t.Skip call: change "techincal" to
"technical" in the sentence referencing the technical preview and the Jira
ticket so the comment reads correctly; the change is in the comment surrounding
the t.Skip("Skipping KMS encryption test: operator implementation is
incomplete") statement in encryption_kms.go.

test/library/encryption/perf_helpers.go (1)

118-120: Misleading log message when workers encounter errors.

The log message "All workers completed successfully" is printed regardless of whether workers encountered errors. Since errors are collected in r.errorStore rather than causing the function to fail, this message can be misleading during debugging.

📝 Suggested improvement

 	r.wg.Wait()
-	t.Log("All workers completed successfully")
+	if len(r.errorStore) > 0 {
+		t.Logf("All workers completed with %d unique error types", len(r.errorStore))
+	} else {
+		t.Log("All workers completed successfully")
+	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/library/encryption/perf_helpers.go` around lines 118 - 120, The current
code always logs "All workers completed successfully" after r.wg.Wait() which is
misleading because worker errors are stored in r.errorStore; update the
post-wait logic to inspect r.errorStore (e.g., check its error count or
equivalent method on the runner) and only emit the success message when it is
empty, otherwise log or fail the test with the collected errors (use
t.Logf/t.Fatalf and include the contents of r.errorStore) so the message
accurately reflects worker outcomes.

test/e2e-encryption-rotation/encryption_rotation.go (1)

76-76: Use the predefined constant configv1.EncryptionTypeAESCBC instead of string cast.

Line 76 uses configv1.EncryptionType("aescbc"), which works but should use the constant configv1.EncryptionTypeAESCBC defined in the API types. This is consistent with how the constant is used throughout the encryption test library and aligns with the codebase pattern.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/e2e-encryption-rotation/encryption_rotation.go` at line 76, Replace the
string-cast usage configv1.EncryptionType("aescbc") with the predefined constant
configv1.EncryptionTypeAESCBC in the EncryptionProvider field; locate the
assignment in encryption_rotation.go (the EncryptionProvider assignment) and
swap the value to configv1.EncryptionTypeAESCBC so the test uses the API
constant consistently with other tests.

test/e2e-oidc/external_oidc.go (3)

950-969: Consider cleaning up test-created Keycloak resources.

The test creates a group and user in Keycloak but doesn't explicitly clean them up. If the Keycloak instance persists across test runs, these resources accumulate. If the IDP cleanup (test.AddKeycloakIDP cleanup handlers at line 127) removes the entire Keycloak deployment, this is acceptable.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/e2e-oidc/external_oidc.go` around lines 950 - 969, The test creates
Keycloak resources (group and user via kcClient.CreateGroup and
kcClient.CreateUser assigned to variables group and user) but never explicitly
deletes them; add cleanup logic that calls the appropriate
kcClient.DeleteGroup(group) and kcClient.DeleteUser(user) (or equivalent
deletion methods) in a deferred or test cleanup handler after creation, or
ensure these deletions are registered with the existing test.AddKeycloakIDP
cleanup handlers so the group and user are removed when the Keycloak IDP is torn
down.

---

390-439: Use context-aware HTTP requests for cancellation support.

The client.Get() calls don't support context cancellation. Consider accepting a context parameter and using http.NewRequestWithContext with client.Do() instead. This allows the requests to be cancelled if the test context times out.

♻️ Proposed fix

-func fetchIssuerJWKS(issuerURL string) (*jwks, error) {
+func fetchIssuerJWKS(ctx context.Context, issuerURL string) (*jwks, error) {
 	client := &http.Client{
 		Timeout: 30 * time.Second,
 		Transport: &http.Transport{
 			TLSClientConfig: &tls.Config{
 				InsecureSkipVerify: true,
 				MinVersion:         tls.VersionTLS12,
 			},
 		},
 	}

 	// grab openid-configuration JSON which contains the URL of the provider's JWKS
-	resp, err := client.Get(issuerURL + "/.well-known/openid-configuration")
+	req, err := http.NewRequestWithContext(ctx, http.MethodGet, issuerURL+"/.well-known/openid-configuration", nil)
+	if err != nil {
+		return nil, fmt.Errorf("could not create request for well-known config: %v", err)
+	}
+	resp, err := client.Do(req)
 	if err != nil {
 		return nil, fmt.Errorf("could not get issuer OpenID well-known configuration: %v", err)
 	}
 	defer resp.Body.Close()
 	// ... similar change for the second request at line 422

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/e2e-oidc/external_oidc.go` around lines 390 - 439, fetchIssuerJWKS
currently uses client.Get which doesn't support cancellation; change its
signature to accept ctx context.Context (e.g., fetchIssuerJWKS(ctx
context.Context, issuerURL string) (*jwks, error)) and replace both client.Get
calls with context-aware requests by creating req :=
http.NewRequestWithContext(ctx, "GET",
issuerURL+"/.well-known/openid-configuration", nil) and req =
http.NewRequestWithContext(ctx, "GET", oidcConfig.JwksURL, nil) and use
client.Do(req) for each, preserving existing error handling and defer
resp.Body.Close().

---

690-716: String-based JSON comparison is fragile.

The exact string comparison of auth-config.json will break if field ordering changes in the operator's JSON marshaling (though Go's json.Marshal is currently deterministic by struct field order). Consider unmarshaling both and using semantic comparison via equality.Semantic.DeepEqual for more robust assertions.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/e2e-oidc/external_oidc.go` around lines 690 - 716, The test currently
compares auth-config.json via exact string equality (expectedAuthConfigJSON vs
actualCM.Data["auth-config.json"]) which is fragile; instead unmarshal both the
expectedAuthConfigJSON and actualCM.Data["auth-config.json"] into generic JSON
objects (e.g., map[string]interface{} or interface{}) and use
k8s.io/apimachinery/pkg/api/equality.Semantic.DeepEqual to assert semantic
equality; update the assertion after fetching actualCM (in the loop that calls
tc.kubeClient.CoreV1().ConfigMaps(...).Get) to unmarshal both JSON strings,
check errors with require.NoError, and require.Truef(t,
equality.Semantic.DeepEqual(expectedObj, actualObj), "unexpected
auth-config.json contents in %s/%s: diff=%v", actualCM.Namespace, actualCM.Name,
cmp.Diff(expectedObj, actualObj)) so field ordering changes won’t break the
test.

test/library/keycloakidp.go (1)

246-285: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Authentication requests can still outlive the test context.

KeycloakClientFor still creates an http.Client without a timeout, and AuthenticatePassword still uses http.NewRequest instead of a request-scoped context. The retry loops in AddKeycloakIDP depend on this path, so a stalled Keycloak connection can ignore poll cancellation and hang until the job timeout. Thread ctx into auth and give the client a timeout.

As per coding guidelines, **/*.go: Go security (prodsec-skills): context.Context for cancellation and timeouts.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/library/keycloakidp.go` around lines 246 - 285, KeycloakClientFor
creates an http.Client with no timeout and AuthenticatePassword builds requests
without a context, allowing auth calls to outlive/circumvent caller cancellation
(e.g., the AddKeycloakIDP retry/poll). Fix by threading a context.Context into
AuthenticatePassword (signature change: AuthenticatePassword(ctx
context.Context, ...)), use http.NewRequestWithContext(ctx, ...) and ensure
callers (including AddKeycloakIDP and its retry loop) pass their ctx through;
also set a sensible timeout on the client created in KeycloakClientFor (or
accept a client/timeout param) so in-flight connections are bounded by
caller/cancelation. Ensure TokenURL() and KeycloakClientFor references remain
unchanged except for wiring the ctx and timeout.

Source: Coding guidelines

test/e2e-encryption-rotation/encryption_rotation.go (1)

53-78: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Handle Authentication operator update conflicts with retry.RetryOnConflict.

This Get → mutate → Update path can fail on resourceVersion conflicts and make this serial rotation test flaky when the operator updates the same object concurrently.

Suggested patch

 import (
 	"context"
 	"encoding/json"
 	"testing"
 	"time"
@@
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/client-go/util/retry"
@@
 		ForceRotationFunc: library.StaticEncryptionForceRotation(func(rawUnsupportedEncryptionCfg []byte) error {
 			cs := operatorencryption.GetClients(t)
-			authOperator, err := cs.OperatorClient.Get(ctx, "cluster", metav1.GetOptions{})
-			if err != nil {
-				return err
-			}
-
-			unsupportedConfigAsMap := map[string]interface{}{}
-			if len(authOperator.Spec.UnsupportedConfigOverrides.Raw) > 0 {
-				if err := json.Unmarshal(authOperator.Spec.UnsupportedConfigOverrides.Raw, &unsupportedConfigAsMap); err != nil {
-					return err
-				}
-			}
-			unsupportedEncryptionConfigAsMap := map[string]interface{}{}
-			if err := json.Unmarshal(rawUnsupportedEncryptionCfg, &unsupportedEncryptionConfigAsMap); err != nil {
-				return err
-			}
-			if err := unstructured.SetNestedMap(unsupportedConfigAsMap, unsupportedEncryptionConfigAsMap, oauthapiconfigobservercontroller.OAuthAPIServerConfigPrefix); err != nil {
-				return err
-			}
-			rawUnsupportedCfg, err := json.Marshal(unsupportedConfigAsMap)
-			if err != nil {
-				return err
-			}
-			authOperator.Spec.UnsupportedConfigOverrides.Raw = rawUnsupportedCfg
-
-			_, err = cs.OperatorClient.Update(ctx, authOperator, metav1.UpdateOptions{})
-			return err
+			return retry.RetryOnConflict(retry.DefaultRetry, func() error {
+				authOperator, err := cs.OperatorClient.Get(ctx, "cluster", metav1.GetOptions{})
+				if err != nil {
+					return err
+				}
+
+				unsupportedConfigAsMap := map[string]interface{}{}
+				if len(authOperator.Spec.UnsupportedConfigOverrides.Raw) > 0 {
+					if err := json.Unmarshal(authOperator.Spec.UnsupportedConfigOverrides.Raw, &unsupportedConfigAsMap); err != nil {
+						return err
+					}
+				}
+				unsupportedEncryptionConfigAsMap := map[string]interface{}{}
+				if err := json.Unmarshal(rawUnsupportedEncryptionCfg, &unsupportedEncryptionConfigAsMap); err != nil {
+					return err
+				}
+				if err := unstructured.SetNestedMap(unsupportedConfigAsMap, unsupportedEncryptionConfigAsMap, oauthapiconfigobservercontroller.OAuthAPIServerConfigPrefix); err != nil {
+					return err
+				}
+				rawUnsupportedCfg, err := json.Marshal(unsupportedConfigAsMap)
+				if err != nil {
+					return err
+				}
+				authOperator.Spec.UnsupportedConfigOverrides.Raw = rawUnsupportedCfg
+
+				_, err = cs.OperatorClient.Update(ctx, authOperator, metav1.UpdateOptions{})
+				return err
+			})
 		}),

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/e2e-encryption-rotation/encryption_rotation.go` around lines 53 - 78,
The Get→mutate→Update sequence on the Authentication operator
(cs.OperatorClient.Get / authOperator / cs.OperatorClient.Update) can fail with
resourceVersion conflicts; wrap the whole read-modify-update in a
retry.RetryOnConflict(retry.DefaultRetry, func() error { ... }) loop that
re-reads the authOperator inside the closure, applies the same JSON
Unmarshal/SetNestedMap/Marshal mutation to
authOperator.Spec.UnsupportedConfigOverrides.Raw, and then calls Update; return
the Update error from the closure and return the retry wrapper result from the
calling function so conflicts are retried safely.

test/library/waits.go (1)

61-70: 🏗️ Heavy lift

Pass a caller context through this wait chain.

WaitForClusterOperatorStatus still hardcodes context.TODO(), so WaitForOperatorToPickUpChanges and the other wrappers can keep polling for the full 10 minutes after the enclosing test has already been canceled. Thread ctx through this helper family and use it in both PollUntilContextTimeout and CheckClusterOperatorStatus.

As per coding guidelines, **/*.go: Go security (prodsec-skills): context.Context for cancellation and timeouts.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/library/waits.go` around lines 61 - 70, WaitForClusterOperatorStatus
currently uses context.TODO() causing callers like
WaitForOperatorToPickUpChanges to be unable to cancel the wait; change the
signature of WaitForClusterOperatorStatus to accept a context.Context parameter,
thread that ctx through the call to wait.PollUntilContextTimeout instead of
context.TODO(), and pass the same ctx into CheckClusterOperatorStatus (and
update any callers such as WaitForOperatorToPickUpChanges to forward their ctx);
ensure all references to PollUntilContextTimeout and CheckClusterOperatorStatus
use the passed ctx so the wait honors caller cancellation.

Source: Coding guidelines

test/library/client.go (1)

168-171: ⚡ Quick win

Accept a caller context for namespace creation.

This helper hardcodes context.Background(), even though callers already track test contexts (test/library/idpdeployment.go:50-59). If the API call stalls, namespace creation ignores suite cancellation. Adding ctx context.Context here keeps the new exported builder consistent with the rest of the context-aware helpers.

As per coding guidelines, **/*.go: Go security (prodsec-skills): context.Context for cancellation and timeouts.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/library/client.go` around lines 168 - 171, Change
TestNamespaceBuilder.Create to accept a caller context (add ctx context.Context
to the signature of Create) and pass that ctx into kubeClient.Create instead of
using context.Background(); update the callsite(s) that use Create (e.g., in
idpdeployment helpers) to forward their test context, and ensure the file
imports context if not already present and the function name
TestNamespaceBuilder.Create and the kubeClient.Create(...) invocation are
updated accordingly.

Source: Coding guidelines

test/e2e-encryption-perf/encryption_perf.go (1)

79-79: ⚡ Quick win

Prefer configv1.EncryptionTypeAESCBC over string casting.

Line 79 should use the typed constant for stronger API compatibility and typo safety.

Proposed change

-			APIServerEncryption: configv1.APIServerEncryption{Type: configv1.EncryptionType("aescbc")},
+			APIServerEncryption: configv1.APIServerEncryption{Type: configv1.EncryptionTypeAESCBC},

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/e2e-encryption-perf/encryption_perf.go` at line 79, Replace the
string-cast encryption type with the typed constant: in the APIServerEncryption
struct (symbol APIServerEncryption / type configv1.APIServerEncryption) change
usage of configv1.EncryptionType("aescbc") to the exported constant
configv1.EncryptionTypeAESCBC to ensure type safety and avoid typos.

test/e2e-encryption/encryption.go (1)

76-76: ⚡ Quick win

Use the typed encryption constant instead of a raw string cast.

Line 76 should prefer the API constant to avoid typo drift and keep enum usage explicit.

Proposed change

-			APIServerEncryption: configv1.APIServerEncryption{Type: configv1.EncryptionType("aescbc")},
+			APIServerEncryption: configv1.APIServerEncryption{Type: configv1.EncryptionTypeAESCBC},

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/e2e-encryption/encryption.go` at line 76, Replace the raw string cast
used for APIServerEncryption (configv1.APIServerEncryption{Type:
configv1.EncryptionType("aescbc")}) with the typed enum constant (e.g.
configv1.AESCBC) to avoid typo drift; update the value to
configv1.APIServerEncryption{Type: configv1.AESCBC} so the code uses the
declared constant instead of a literal string cast.

test/e2e-encryption-rotation/encryption_rotation.go (1)

49-49: ⚡ Quick win

Use the typed AES-CBC constant instead of a string cast.

configv1.EncryptionTypeAESCBC is clearer and avoids literal drift.

Suggested patch

-			APIServerEncryption: configv1.APIServerEncryption{Type: configv1.EncryptionType("aescbc")},
+			APIServerEncryption: configv1.APIServerEncryption{Type: configv1.EncryptionTypeAESCBC},

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/e2e-encryption-rotation/encryption_rotation.go` at line 49, Replace the
string-cast encryption type with the typed constant: in the APIServerEncryption
initialization (configv1.APIServerEncryption) use configv1.EncryptionTypeAESCBC
instead of configv1.EncryptionType("aescbc") so the field reads
APIServerEncryption: configv1.APIServerEncryption{Type:
configv1.EncryptionTypeAESCBC}.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes